Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

Adds swift package manager manifest support (one of many) #354

Merged
merged 9 commits into from
Sep 28, 2021

Conversation

meghfossa
Copy link
Contributor

@meghfossa meghfossa commented Sep 1, 2021

Overview

Adds analyses for swift package manager manifest.

Acceptance criteria

  • Swift Package Manager's manifest file can be analyzed, and dependencies can be reported.

Testing plan

  • Create an example swift project managed by swift package manager (or clone one)
  • Perform fossa analyze -o, and inspect the result.

To perform, end to end analysis,

Risks

N/A

References

Works on https://github.com/fossas/team-analysis/issues/711

Checklist

  • I added tests for this PR's change (or confirmed tests are not viable).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • I updated Changelog.md if this change is externally facing. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • I linked this PR to any referenced GitHub issues, if they exist.

@meghfossa
Copy link
Contributor Author

Leaving change-log till approval. Will only merge after fetcher PR is merged.

@meghfossa meghfossa requested review from a team and skilly-lily and removed request for a team September 1, 2021 01:55
Copy link
Contributor

@skilly-lily skilly-lily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One major structural question about SwiftPackageGitDep, let dial that in before more review. Added some other comments as well.

src/Strategy/Swift/PackageSwift.hs Outdated Show resolved Hide resolved
src/Strategy/Swift/PackageSwift.hs Outdated Show resolved Hide resolved
src/Strategy/Swift/PackageSwift.hs Outdated Show resolved Hide resolved
src/Strategy/Swift/PackageSwift.hs Outdated Show resolved Hide resolved
@meghfossa
Copy link
Contributor Author

meghfossa commented Sep 3, 2021

One major structural question about SwiftPackageGitDep, let dial that in before more review. Added some other comments as well.

Addressed in 82b04de

@meghfossa meghfossa requested review from a team and zlav and removed request for a team September 7, 2021 17:30
Copy link
Member

@zlav zlav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great but I have a few questions around SwiftType and version constraints

docs/strategies/ios/swift.md Outdated Show resolved Hide resolved
docs/strategies/ios/swift.md Outdated Show resolved Hide resolved
docs/strategies/ios/swift.md Show resolved Hide resolved
src/DepTypes.hs Show resolved Hide resolved
Comment on lines +118 to +130
toConstraint (From f) = CEq $ "^" <> f
toConstraint (UpToNextMajor c) = CEq $ "^" <> c
toConstraint (UpToNextMinor c) = CEq $ "~" <> c
toConstraint (ClosedInterval (lhs, rhs)) = CEq $ ">=" <> lhs <> " " <> "<=" <> rhs
toConstraint (RhsHalfOpenInterval (lhs, rhs)) = CEq $ ">=" <> lhs <> " " <> "<" <> rhs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure how to handle these version constraint types, but we're using CEq incorrectly here. I think for From you can use CCompatible but we don't have an equivalent for the others yet...

I'm split between saying

  • Strip all of the version constraints and leave the version as CEq c or CEq rhs/lhs to ensure that the FOSSA backend can handle the version correctly.
  • Leave it the way you have it because its more correct.

Can you test what happens when a locator is sent to the backed with a RhsHalfOpenInterval? I'm concerned that the result won't be what we want. It may be better to do CEq lhs with a note about how we have an issue. You should be able to make a fossa-deps file, type: git, name: <some swift package with the valid constraint>.

[future-work] Alternatively, you could add a type like CRhsHalfOpenInterval to VerConstraint so that we handle this properly when the graph is converted to SrcUnits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is we go with (2) leave it way it is because it is more correct.

If (1) is followed, we might fail to resolve versions when LHS or RHS of constraint does not exist (not a perfect example but ~ e.g. >= 0.0.0, where 0.0.0 does not exist)

I've validated both RhsHalfOpenInterval, ClosedInterval, and others in unit and integration test for swift fetchers:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm comfortable with option 2 as well, mainly because of our conversation yesterday about how the Swift fetcher is built to handle these constraints.

Could we actually add a type similar to CRhsHalfOpenInterval and others for this PR? I think that would ease all of my concerns about version handling and it shouldn't be a hard addition. We're going to need it eventually and I think this VersionConstraint would make sure we always handle this version case the same everywhere.

src/Strategy/Swift/PackageSwift.hs Show resolved Hide resolved
@meghfossa meghfossa requested a review from zlav September 9, 2021 02:48
Copy link
Member

@zlav zlav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed answers! Looks great.

@meghfossa meghfossa force-pushed the feat/swift-pm-support branch from 13a09fb to 08db396 Compare September 28, 2021 19:38
@meghfossa meghfossa merged commit 47edc9f into master Sep 28, 2021
@meghfossa meghfossa deleted the feat/swift-pm-support branch September 28, 2021 20:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants